chore(db): drop the legacy folder tables and adopt the deferred folder_id FKs by waleedlatif1 · Pull Request #6051 · simstudioai/sim

waleedlatif1 · 2026-07-29T17:08:59Z

Summary

Contract migration closing out the generic-folders cutover: drops workflow_folder and workspace_file_folders, and adopts the two folder_id foreign keys 0272 deliberately deferred.
Runs a final insert-only reconcile first, so no stranded legacy folder is lost by the drop. In production this is a no-op (verified 0 stranded); it exists for deployments that never ran the post-drain reconcile as an operational step — self-hosted upgrades above all, where a rolling restart strands folders the same way and nobody is watching for it.
Removes both table definitions from schema.ts and the testing schema mock.
Re-roots active children before the retention sweep hard-deletes a folder (see "FK side effect" below).

The thing to review closely: three re-root paths, three unique indexes

The migration re-roots rows in three places, and every destination is covered by a partial unique index keyed on a coalesced nullable column — so a name that is free in the source scope may already be taken in the destination:

index	key
`folder_workspace_resource_parent_name_active_unique`	`(workspace_id, resource_type, coalesce(parent_id,''), name)`
`workflow_workspace_folder_name_active_unique`	`(workspace_id, coalesce(folder_id,''), name)`
`workspace_files_workspace_folder_name_active_unique`	`(workspace_id, coalesce(folder_id,''), original_name)`

Three bugs were found and fixed here (all would abort the migration; all are 0-row in production and reachable on self-hosted):

Step 1's free-suffix probe consulted only the folder table. Stranded rows are by definition absent from it, so a stranded row keeping the name X (1) was invisible to the probe run for a stranded X — both got assigned X (1). Fixed by also probing the batch's own claimed names (kept).
Step 2 re-rooted dangling folder_ids with no dedupe at all, colliding on the workflow / workspace_files indexes. Note the asymmetry that gave it away: step 1 dedupes meticulously when it re-roots, step 2 didn't.
A stranded folder whose parent lives in another workspace passed the parent resolver but fails the folder_parent_resource_type_match trigger (the legacy self-FK never checked workspace). Now re-roots on mismatch.

Also: the stranded-row guard now keys on id alone, matching the primary key it protects, and suffix exhaustion falls back to the row id instead of the base name (which would guarantee a collision and report a misleading error).

FK side effect worth calling out

Adopting ON DELETE SET NULL changes what happens when the retention sweep hard-deletes a folder: previously the child's folder_id was left dangling (invisible in the sidebar), now Postgres re-roots it — an improvement, except it can collide on those same indexes, and chunkedBatchDelete turns a 23505 into hasMore = false, permanently stalling folder retention for that workspace chunk. reRootActiveFolderChildren now renames such children before the delete, leaving the SET NULL a no-op. Only workflow and workspace_files have these indexes; KB and tables do not.

Why the migration is hand-written

drizzle-kit generate produces a correct-looking version that is unsafe to run:

It drops the tables before adding the FKs, with no step to NULL unresolvable folder_ids. One dangling id makes ADD CONSTRAINT fail after the legacy tables are already gone — unrecoverable.
It adds the FKs with immediate validation. workspace_files is ~1.7M rows / 1.8GB, so that holds ACCESS EXCLUSIVE across a full scan of a hot table. This uses NOT VALID → COMMIT → VALIDATE, which takes only SHARE UPDATE EXCLUSIVE.
It uses DROP TABLE ... CASCADE. Nothing references either table, so this drops them plainly and lets the absence of CASCADE act as the safety check.

The generated snapshot is kept, so drizzle-kit generate reports "No schema changes".

Production preconditions (verified read-only before writing the migration)

0 stranded rows in either tree; 0 unresolvable folder_ids; 0 active rows filed under a soft-deleted folder.
FULL-ROW comparison (name, parent, deleted/archived state, workspace, user, sort order, locked) clean on both trees — a row count is not sufficient, since the pre-0274 divergence was in row contents.
The only divergence is 47 workflow-folder names, all matching <old name> (N) (verified by regex shape, not by the count matching) — 0272's deliberate dedup renames. That is exactly why the reconcile is insert-only: an upsert would revert all 47.

Testing

Executed end-to-end against local Postgres 17.9. The fixture is first run against the pre-fix migration to prove it reproduces all three bugs (it fails on Batch (1), Shared, and report.pdf), then against the fixed version. Asserted:

batch-internal suffix collision → Batch, Batch (1), Batch (2) (skips the suffix another batch row claims)
root-collision re-root → Shared / Shared (1), and two danglers from different ghost folders → Twin / Twin (1)
an archived dangler is re-rooted but never renamed (index is partial), coexisting with an active row of the same name
cross-workspace parent → re-rooted to NULL
stranded locked folder → locked preserved; stranded child of stranded parent → parent preserved
soft-deleted collider kept verbatim; already-mirrored rows untouched including a 0272 rename (insert-only confirmed)
FK rejects a dangling id; ON DELETE SET NULL fires; workflow_folder_sort_idx (an index on the live workflow table) survives the drop
re-running the whole file is a clean no-op with no double-suffixing — replay-safe

Typecheck, check:migrations, check:api-validation, lint, and 1196 tests pass.

Type of Change

Chore / migration

Checklist

Code follows project style guidelines
Self-reviewed my changes
Tests added/updated and passing
No new warnings introduced
I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

vercel · 2026-07-29T17:09:02Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
docs	Ready	Preview, Comment	Jul 29, 2026 7:22pm

cursor · 2026-07-29T17:09:09Z

PR Summary

High Risk
Irreversible schema migration on hot tables (~1.7M workspace_files) plus runtime mutations to surviving rows during cleanup; mistakes could fail deploys or permanently stall folder retention.

Overview
Closes the generic-folders cutover with migration 0276: insert-only reconcile from workflow_folder / workspace_file_folders into folder, re-root dangling folder_ids with batch-aware name dedup (fixes collision bugs that would abort deploy), add workflow and workspace_files folder_id FKs via NOT VALID → COMMIT → VALIDATE, then drop the legacy tables.

Because those FKs use ON DELETE SET NULL, hard-deleting an expired folder can collide on partial unique indexes (coalesce(folder_id,'') / parent_id) and stall folder retention. The soft-delete sweep now runs an onBatch hook (wired through batchDeleteByWorkspaceAndTimestamp) that re-roots and renames active workflows, workspace files, and child folders before the delete, with id-suffixed fallbacks when allocators fail. allocateUniqueWorkspaceFileName is exported for that path.

New tests cover the re-root hook and assert onBatch runs before DELETE.

^{Reviewed by Cursor Bugbot for commit 797bdce. Configure here.}

greptile-apps · 2026-07-29T17:14:05Z

Greptile Summary

Closes the generic-folders cutover by dropping legacy folder tables and adopting deferred folder_id FKs.

Adds migration 0276 with insert-only stranded reconcile, dangling folder_id re-root/dedup, NOT VALID then VALIDATE FKs, and plain DROP TABLE of workflow_folder / workspace_file_folders.
Removes legacy tables from schema.ts and the testing schema mock.
Re-roots active workflows, workspace files, and child folders (with name dedup) via onBatch before hard-deleting folders so ON DELETE SET NULL cannot hit partial unique indexes and stall retention.

Confidence Score: 5/5

This PR appears safe to merge; the prior stranded name-dedup collision is addressed and no blocking failure remains.

The stranded active-sibling suffix probe now excludes batch-kept names as well as existing folder rows on both legacy trees, and no remaining blocking failure was identified on the follow-up pass.

Important Files Changed

Filename	Overview
packages/db/migrations/0276_drop_legacy_folder_tables.sql	Hand-written cutover migration with stranded reconcile, dangling re-root/dedup (including kept-name probes), deferred FK validation, and legacy table drops.
apps/sim/background/cleanup-soft-deletes.ts	Pre-delete re-root of active folder children with fallback unique names so SET NULL cannot 23505-stall retention.
apps/sim/lib/cleanup/batch-delete.ts	Adds onBatch hook wired before each chunk delete for pre-delete side effects.
packages/db/schema.ts	Drops legacy workflow_folder and workspace_file_folders definitions and adopts folder_id FKs in schema.
apps/sim/background/cleanup-soft-deletes.test.ts	Covers re-root paths, allocator failure fallbacks, restore race, and empty batch behavior.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Step1 insert-only reconcile stranded legacy folders] --> B[Step2 re-root dangling folder_id with name dedup]
  B --> C[Step3 ADD FK NOT VALID]
  C --> D[COMMIT]
  D --> E[VALIDATE FK]
  E --> F[DROP legacy workflow_folder and workspace_file_folders]
  G[Retention hard-delete folder batch] --> H[onBatch reRootActiveFolderChildren]
  H --> I[DELETE folder; SET NULL is no-op]

_{Reviews (8): Last reviewed commit: "fix(db): stop the re-root dedupe renamin..." | Re-trigger Greptile}

waleedlatif1 · 2026-07-29T17:28:46Z

@greptile

waleedlatif1 · 2026-07-29T17:28:47Z

@cursor review

waleedlatif1 · 2026-07-29T17:31:17Z

The 3/5 finding is real, and it was fixed in eeda0c12f — the review ran against fffb0ec9, two commits back.

Greptile's exact scenario: base name taken in folder, plus a stranded row already holding the app-style (N) suffix. The cause was that step 1's free-suffix probe consulted only the folder table — and stranded rows are by definition absent from it, so a stranded row about to keep the name X (1) was invisible to the probe running for a stranded X, and both were assigned X (1). 0272's equivalent probed the source table and did cover its own batch; that coverage was lost in the rewrite.

The fix adds a kept CTE — the batch rows whose slot is < 0, i.e. the ones keeping their base name — and the probe now requires a candidate to be free in both folder and kept.

Verified by execution against Postgres 17.9, not by reading. The fixture is first run against the pre-fix migration to confirm it reproduces the abort:

ERROR:  duplicate key value violates unique constraint "folder_workspace_resource_parent_name_active_unique"
DETAIL:  Key (workspace_id, resource_type, COALESCE(parent_id, ''::text), name)=(ws1, workflow, , Batch (1)) already exists.

Then against the fix, for Greptile's precise combination (folder holds active Mix; stranded Mix and stranded Mix (1)):

    id    |  name
----------+---------
 mix-base | Mix
 s-mix    | Mix (2)
 s-mix1   | Mix (1)

Two further bugs of the same class were found and fixed in the same commit, since all three destinations are partial unique indexes keyed on a coalesced nullable column:

step 2 re-rooted dangling folder_ids to NULL with no dedup, colliding on workflow_workspace_folder_name_active_unique / workspace_files_workspace_folder_name_active_unique
a stranded folder whose parent lives in another workspace passed the parent resolver but fails the folder_parent_resource_type_match trigger

Re-running review against current HEAD.

waleedlatif1 · 2026-07-29T17:31:19Z

@greptile

waleedlatif1 · 2026-07-29T17:31:20Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit eeda0c1. Configure here.}

waleedlatif1 · 2026-07-29T17:54:13Z

@greptile

waleedlatif1 · 2026-07-29T17:54:14Z

@cursor review

waleedlatif1 · 2026-07-29T18:10:35Z

@greptile

waleedlatif1 · 2026-07-29T18:10:36Z

@cursor review

waleedlatif1 · 2026-07-29T18:25:42Z

@greptile

waleedlatif1 · 2026-07-29T18:25:43Z

@cursor review

waleedlatif1 · 2026-07-29T18:42:23Z

@greptile

waleedlatif1 · 2026-07-29T18:42:24Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 461ddd4. Configure here.}

…r_id FKs Contract migration closing out the generic-folders cutover: drops workflow_folder and workspace_file_folders, and adopts the two folder_id foreign keys 0272 deliberately deferred. Runs a final INSERT-ONLY reconcile first so no stranded legacy folder is lost by the drop. Insert-only because 0272 deliberately renamed 47 workflow folders to dedupe them, and an upsert would revert all 47. In production this is a no-op (verified 0 stranded); it exists for deployments that never ran the post-drain reconcile as an operational step, self-hosted upgrades above all. Hand-written rather than drizzle-generated: the generated form drops the tables BEFORE adding the FKs with no step to re-root unresolvable folder_ids, so one dangling id fails ADD CONSTRAINT after the legacy data is already gone; it also validates the FK inline, holding ACCESS EXCLUSIVE across a full scan of the 1.7M-row workspace_files, and uses DROP TABLE CASCADE. The generated snapshot is kept so drizzle-kit generate reports no schema changes. Every re-root path dedupes, because all three destinations are partial unique indexes keyed on a coalesced nullable column: folder, workflow, and workspace_files. Parents must also be reachable — an active row re-roots off a soft-deleted parent, while a soft-deleted row may keep one. Also hardens the retention sweep, which the new ON DELETE SET NULL exposes: a surviving active child re-rooted by the FK can collide at the workspace root, and chunkedBatchDelete turns that 23505 into a permanent per-chunk stall. The folder cleanup target now renames children first, covering workflows, files and subfolders, re-asserts eligibility so a restore mid-batch is not stripped, and treats the deduplicated name as a hint since both allocators can return a colliding one.

waleedlatif1 · 2026-07-29T19:12:31Z

Rebased onto latest staging and renumbered 0275 → 0276: staging landed its own 0275_table_views.sql (#5961) while this was in review, so both files claimed the same index. The migration content is byte-identical to what was reviewed; only the filename, the journal tag, and the snapshot changed. The snapshot was regenerated from the post-table-views schema rather than renamed, so it builds on staging's 0275 — drizzle-kit generate reports "No schema changes".

Squashed to one commit because the rebase conflicted only in _journal.json and meta/0275_snapshot.json; replaying four commits through that would have produced four throwaway resolutions. My app-code changes had zero overlap with staging's — the only hand-merged file was schema.ts, where staging added table-views tables alongside my removals.

Also folded in two fixes from a fresh review pass:

Child folders hit the same 23505. folder.parentId is itself ON DELETE SET NULL and folder_workspace_resource_parent_name_active_unique keys on coalesce(parent_id,''), so purging a parent can collide a surviving active subfolder at the root. The hook covered workflows and files only, while its TSDoc presented the class as closed.
The hook's own SELECTs were unguarded, so a transient read failure rejected onBatch and produced exactly the aborted batch it exists to prevent.

And the first tests for lib/cleanup: nothing verified that onBatch survives batchDeleteByWorkspaceAndTimestamp's ...rest hop, or that chunkedBatchDelete runs it BEFORE the delete. Every other test invokes the captured callback directly with that module mocked, so a regression in either link would have left the suite green. Both assertions are mutation-checked.

Gates on the rebuilt branch: typecheck, check:migrations, check:api-validation, lint, and 1765 tests across 118 files (now including staging's table-views suites).

waleedlatif1 · 2026-07-29T19:12:32Z

@greptile

waleedlatif1 · 2026-07-29T19:12:33Z

@cursor review

workflow.workspace_id is nullable, and NULL is treated as EQUAL by PARTITION BY but UNKNOWN by the = in base_taken. So for personal workflows rn incremented across the group while no collision was ever detected: the dedupe could only fire spuriously, renaming a user-visible workflow that needed no rename, since the unique index treats NULL workspace_id rows as distinct anyway. The file block already carried the equivalent guard. Also gives step 1's inserts ON CONFLICT (id) DO NOTHING. It restates the stranded guard, closing the window between that read's snapshot and the index check — an operational re-run of 0274, or a live pod, committing into folder mid-statement would otherwise raise 23505, and migrate.ts retries only 55P03. 0272 and 0274 were already written this way; step 1 was the exception. Corrects three comments that claimed more than the code delivers: the header's 'no legacy folder is lost' (an id already present under another resource_type cannot be rescued), the reachability note (a cycle among stranded rows survives a per-row parent check), and a note made stale by the ON CONFLICT above.

waleedlatif1 · 2026-07-29T19:18:55Z

A cold adversarial review of the migration (500 fuzz iterations against a throwaway Postgres 17) found one real bug and two hardening gaps. Fixed in 797bdcee7.

Real bug — the re-root dedupe renamed personal workflows for no reason. workflow.workspace_id is nullable, and NULL is treated as equal by PARTITION BY but as unknown by the = in base_taken. So across personal workflows rn incremented while no collision was ever detected: the dedupe could only fire spuriously. The unique index treats NULL-workspace rows as distinct, so no rename was ever required. The file block already had the equivalent guard; the workflow block didn't.

Differential run — pre-fix vs post-fix, three personal workflows all named My Flow:

PRE-FIX                              POST-FIX
 p0 |  |  | My Flow                    p0 |  |  | My Flow
 p1 |  |  | My Flow      <- still dup  p1 |  |  | My Flow
 p2 |  |  | My Flow (1)  <- gratuitous p2 |  |  | My Flow

The rename achieved nothing (p1 and p0 still share the name) while mutating p2.

Hardening — step 1's inserts now carry ON CONFLICT (id) DO NOTHING. That restates the stranded guard and closes the window between its snapshot and the index check: an operational re-run of 0274, or a live pod, committing into folder mid-statement would otherwise raise 23505 — and migrate.ts retries only 55P03, so it hard-fails the deploy rather than replaying. 0272 and 0274 were already written this way; step 1 was the lone exception.

Three comments corrected for overclaiming, which has been the recurring defect in this file:

the header's "no legacy folder is lost by the DROP" — a legacy id already present under a different resource_type cannot be inserted (pkey taken) and is dropped; not reachable in practice, but the wording was wrong
the reachability note — a cycle among stranded rows survives a per-row parent check. 0272's backfill has the identical hole and the client carries cycle guards, so it is documented as a deliberate limitation rather than fixed
one note made stale by the ON CONFLICT above

Clean per the same review: the slot arithmetic at all four dedupe sites, the parent resolver across all six cases, step 2's UPDATE ... FROM, replay from every restart point including a partial-drop replay, fresh-install behaviour, and DROP TABLE without CASCADE.

Also renumbered 0275 → 0276 earlier in this round; staging landed its own 0275_table_views.sql mid-review.

waleedlatif1 · 2026-07-29T19:18:56Z

@greptile

waleedlatif1 · 2026-07-29T19:18:58Z

@cursor review

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 797bdce. Configure here.}

File folders already live in the shared folder table as resourceType 'file' (#6045 cut them over, #6051 dropped workspace_file_folders), and the remaining file-specific folder machinery is being folded into the generic folder engine. Publishing /api/v2/files/folders/** would pin that transitional split into a public contract we'd then have to keep or break. Files stay folder-aware — folderId/folderPath on the projection, folderId on upload, and the move route — because a folder id is a folder.id and survives the unification untouched. Folder management belongs on /api/v2/folders once that surface serves resourceType 'file'; until then there is no v2 way to enumerate file folders, which is the deliberate gap. The orchestration classification fixes stay: the internal routes and the copilot file-folder tools still call those perform* functions.

* feat(api): expand the public v2 files surface Adds folder support, rename/restore, move, bulk archive, share, and content replace to /api/v2/files, so managing files by API no longer stops at upload + download + archive-one. Routes are thin: auth -> parse -> perform* -> serialize. Share and content replace get their orchestration extracted first so the session routes and the public ones cannot diverge on the effective-authType resolution, the EE public-sharing gate, or the storage-quota classification. Presigned upload stays session-only: presign does an advisory quota check and the real debit happens in the separate register step, so a caller that never registers leaves unaccounted bytes with no reaper. The buffered multipart path debits inside uploadWorkspaceFile's own transaction. * fix(files): classify folder and content failures instead of 500ing them Bugbot round 1. The v2 routes map errorCode straight to a status, so every manager failure that arrived unclassified became a 500 for what is really a caller-fixable 400 or 404. - Folder manager throws OrchestrationError: missing target/folder -> not_found, reparent cycle / self-parent / restore-into-archived-workspace -> validation. - File manager does the same for the in-transaction 'File not found' paths that the earlier pass missed. - updateWorkspaceFileContent's outer catch re-wrapped everything in a bare Error, which stripped the class off StorageLimitExceededError and the new not_found alike. It now rethrows a classified failure untouched and attaches cause to the generic wrap, so asOrchestrationError can still walk the chain. - Every remaining perform* gained the asOrchestrationError branch. - renameWorkspaceFile returned the pre-update read, so the v2 PATCH reported a stale updatedAt; it now returns the timestamp it actually wrote. Docs: upload auto-suffixes a duplicate name rather than rejecting it, matching the in-app uploader. The description claimed 409 and was simply wrong. * fix(files): surface a failed upload read-back as the real error getWorkspaceFile swallows a query failure and returns null unless throwOnError is set, so a transient blip on the post-upload read reported as 'file could not be read back'. Distinguish the two: a real null after a just-committed write is an invariant break, a query failure is itself. * revert(api): drop the dedicated v2 file-folder routes File folders already live in the shared folder table as resourceType 'file' (#6045 cut them over, #6051 dropped workspace_file_folders), and the remaining file-specific folder machinery is being folded into the generic folder engine. Publishing /api/v2/files/folders/** would pin that transitional split into a public contract we'd then have to keep or break. Files stay folder-aware — folderId/folderPath on the projection, folderId on upload, and the move route — because a folder id is a folder.id and survives the unification untouched. Folder management belongs on /api/v2/folders once that surface serves resourceType 'file'; until then there is no v2 way to enumerate file folders, which is the deliberate gap. The orchestration classification fixes stay: the internal routes and the copilot file-folder tools still call those perform* functions. * fix(files): classify upload failures instead of matching their wording Bugbot round 2. uploadWorkspaceFile had the same outer-catch rewrap that updateWorkspaceFileContent did, so a blown storage quota reached the route as a bare Error and the v2 handler recovered the status by substring-matching the message. Any rewording silently demoted a 413 to a 500. - uploadWorkspaceFile rethrows a classified failure untouched and attaches cause to the generic wrap. - FileConflictError is now an OrchestrationError('conflict'), so a duplicate name classifies like every other conflict. Its 'FILE_EXISTS' discriminator had no readers and is gone; the instanceof checks elsewhere still hold. - The v2 upload handler uses v2CaughtOrchestrationError, dropping all three string matches. Also documents that bulk-archive is best-effort: unknown or already-archived ids are skipped rather than failing the call, and deletedItems is what actually happened. That asymmetry with the single-id DELETE was undocumented.

greptile-apps Bot reviewed Jul 29, 2026

View reviewed changes

Comment thread packages/db/migrations/0276_drop_legacy_folder_tables.sql

vercel Bot deployed to Preview July 29, 2026 17:16 View deployment

waleedlatif1 force-pushed the feat/drop-legacy-folder-tables branch from 9b919b8 to eeda0c1 Compare July 29, 2026 17:28

vercel Bot deployed to Preview July 29, 2026 17:32 View deployment